Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mismatch between CSSInterpolation and Interpolation<Props> #3164

Merged
merged 5 commits into from
Mar 29, 2024

Conversation

Cerber-Ursi
Copy link
Contributor

@Cerber-Ursi Cerber-Ursi commented Mar 5, 2024

What:

Eliminated types mismatch between CSSInterpolation (related to csstypes) and Interpolation<Props> (related to emotion's own styled components machinery), which prevented the former from being used as the latter.

Why:

This fixes #3145.

How:

This required two different changes:

  • Change type of ArrayInterpolation. Since it's expected that every interpolated value is treated by emotion as immutable, this change is correct, and it fixes the mismatch. At the same time, it breaks type inference on styled component creator functions, which required a second change.
  • Change order of overloads for styled.X functions. The idea is that we want to use "template string tag" overload wherever possible and fall back to another one only for the explicit function calls. Previously, these two overloads were independent and the order didn't matter, but now, as every valid TemplateStringsArray is also a valid ArrayInterpolation, we need to explicitly prioritize the "template string tag" case.

Checklist:

  • Documentation (N/A)
  • Tests
  • Code complete
  • Changeset

Formerly, the first overload to be tried was not accepting template strings array as first argument. Therefore, it couldn't be used when `styled` was used as a tag for template string. So in this case TS skipped this overload and fell through to the next.
Now, though, with `ArrayInterpolation` type changed, `TemplateStringsArray` matches the definition of `ArrayInterpolation`; therefore, this overload becomes used for template strings, confusing type inference.

This change moves this overload to the end of the list, i.e. to be used as fallback when there's actually a direct function call, without template string.
Copy link

changeset-bot bot commented Mar 5, 2024

🦋 Changeset detected

Latest commit: 2a76367

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@emotion/serialize Patch
@emotion/styled Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codesandbox-ci bot commented Mar 5, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@mmarcuccio
Copy link

@emmatown do you happen to have time to review this PR?

@mmarcuccio
Copy link

@Andarist do you happen to have time to review this PR? Sorry for repeated tagging, but I don't want this effort to get lost 😄

@@ -52,7 +52,7 @@ export type Keyframes = {
} & string

export interface ArrayInterpolation<Props>
extends Array<Interpolation<Props>> {}
extends ReadonlyArray<Interpolation<Props>> {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes sense to me, it shouldnt hurt us anyhow

@@ -63,10 +63,18 @@ export interface CreateStyledComponent<
SpecificComponentProps extends {} = {},
JSXProps extends {} = {}
> {
(
template: TemplateStringsArray,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see where you are going with this reordering and I think it makes sense. However, I'm really anxious about reordering those overloads. This change needs some extra coverage in our type tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I misunderstood this in part - this is actually a fix for existing tests (that became import after the change in @emotion/serialize)

@Andarist Andarist merged commit c9b84db into emotion-js:main Mar 29, 2024
7 of 8 checks passed
@github-actions github-actions bot mentioned this pull request Mar 29, 2024
@valleywood
Copy link

@Cerber-Ursi @Andarist I believe this PR is related to this ticket 🤔 (haven't been able to verify):
#3174

@Cerber-Ursi
Copy link
Contributor Author

Thanks for notifying, will check soon. Seems that all this ReadonlyArray thing has quite a fallout actually...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSSInterpolation Type is Incompatible with CSS Prop
5 participants